perf(drive-abci): remove historical withdrawal status diagnostic - #4569
Conversation
…ug log line pool_withdrawals_into_transactions_queue_v1 fetches every withdrawal document the chain has ever produced, groups them by status and sorts each group, on any block with nothing queued — nearly every block — and then throws the result away unless debug logging is on. fetch_oldest_withdrawal_documents passes limit: None, and withdrawal documents are never removed, so the cost grows with chain history: measured at 4.8 ms per block by height 200,000 on mainnet and still climbing, against about 7 ms for everything else in a block put together. Do the work only when the line it feeds will be emitted.
|
🕓 Queued for automated review — 61st in line, estimated start in ~57 h (commit edf1fa4)
|
|
Warning Review limit reachedNext included review available in 50 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe withdrawal queue now checks debug logging before fetching historical withdrawal documents. The status-counting branch relies on this earlier check. ChangesWithdrawal queue processing
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change removes an unnecessary history-wide read from the normal no-withdrawal path while preserving withdrawal processing and externally visible behavior. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4569 +/- ##
============================================
- Coverage 87.68% 86.42% -1.27%
============================================
Files 2778 2795 +17
Lines 359190 369084 +9894
============================================
+ Hits 314960 318980 +4020
- Misses 44230 50104 +5874
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — GLM Flash + Sol
The intended default-path performance improvement is real, but the implementation modifies an active versioned block-execution method and makes execution success depend on node-local tracing configuration. The newly conditional DEBUG diagnostics also lack focused coverage.
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-5.6-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-5.6-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-5.6-sol (agent: sol-verifier, role: final-verifier)
Review provenance
- Phase 1 reviewers (GLM Flash):
glm-5.3-flash— general (completed); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed); agentphase1-reviewer - Fresh verifier (Sol):
gpt-5.6-sol— final-verifier; agentsol-verifier - Phase 2 reviewers (Sol):
gpt-5.6-sol— general (completed); agentphase2-reviewer,gpt-5.6-sol— rust-quality (completed); agentphase2-reviewer
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs:58-64: Do not change active versioned block-execution behavior in place
`pool_withdrawals_into_transactions_queue_v1` is selected from protocol v5 onward, including v14 through `DRIVE_ABCI_METHOD_VERSIONS_V10`, while the v0 implementation can also delegate to it. This therefore changes behavior for already-defined protocol versions. More importantly, `fetch_oldest_withdrawal_documents` calls the fallible `query_documents` path and previously propagated its error unconditionally; after this guard, the same block and state can return `Ok(())` on a node without DEBUG tracing while a DEBUG-enabled node or older binary still executes the query and can fail. The successful app-hash replay confirms the normal path but does not cover this changed failure behavior. Keep v0/v1 frozen, introduce a v2 implementation selected at a protocol activation boundary, and make its purely diagnostic query non-fatal so tracing configuration cannot affect block-execution success.
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs:58-70: Exercise the DEBUG-enabled diagnostic path
The existing no-queued-documents test installs no DEBUG subscriber, so it now returns at line 59; `test_pooling` has queued documents and never enters this branch. Consequently, no test executes the guarded historical fetch or the status-grouping branch, matching Codecov's seven uncovered changed lines. Add a focused test using a scoped DEBUG subscriber and at least one non-queued withdrawal document, then assert that the call succeeds and leaves the document unchanged. This covers both the enabled guard and the grouped-status diagnostic path.
| if !tracing::enabled!(tracing::Level::DEBUG) { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let all_documents = self | ||
| .drive | ||
| .fetch_oldest_withdrawal_documents(transaction, platform_version)?; |
There was a problem hiding this comment.
🔴 Blocking: Do not change active versioned block-execution behavior in place
pool_withdrawals_into_transactions_queue_v1 is selected from protocol v5 onward, including v14 through DRIVE_ABCI_METHOD_VERSIONS_V10, while the v0 implementation can also delegate to it. This therefore changes behavior for already-defined protocol versions. More importantly, fetch_oldest_withdrawal_documents calls the fallible query_documents path and previously propagated its error unconditionally; after this guard, the same block and state can return Ok(()) on a node without DEBUG tracing while a DEBUG-enabled node or older binary still executes the query and can fail. The successful app-hash replay confirms the normal path but does not cover this changed failure behavior. Keep v0/v1 frozen, introduce a v2 implementation selected at a protocol activation boundary, and make its purely diagnostic query non-fatal so tracing configuration cannot affect block-execution success.
source: ['claude']
There was a problem hiding this comment.
Partially addressed in efd398de: the diagnostic fetch is no longer fatal. A Drive error in fetch_oldest_withdrawal_documents is logged at DEBUG and the method returns Ok(()), so block outcome no longer depends on the log level. Verified on head cf5e8e4a.
The question of whether this needs a v2 method version rather than an in-place edit to pool_withdrawals_into_transactions_queue_v1 is left for the maintainer to decide; this thread stays open for that.
🤖 Posted autonomously by Claude on behalf of pasta.
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Review
Verdict: merge after two small changes (make the diagnostic fetch non-fatal; cover the DEBUG path with a test so codecov/patch passes).
1. Correctness
The guard is right. fetch_oldest_withdrawal_documents passes limit: None, deserializes every withdrawal document ever written, and the result only feeds a tracing::debug! line. Returning before it when DEBUG is off changes no state; the 425k-block app-hash match confirms that.
One thing the guard does change: the fetch used ?, so a Drive error inside it failed the block. Now a node with DEBUG off returns Ok(()) while a node with DEBUG on still fails the block. The error itself would mean a broken database, so this is theoretical, but tracing configuration should never decide whether a block succeeds. Since the query is purely diagnostic, log the error and continue instead of propagating it. That also resolves thepastaclaw's blocking comment without a new method version: the change is diagnostic-only, touches neither state nor app hash, and does not need protocol activation.
2. Clarity
Title and description are clear. The cost-by-height table makes the case. The code comment explains why, which is what a reader needs.
3. Codebase standards
Follows the tracing::enabled! pattern already used a few lines below. Conventional-commit title with the right scope. No test covers the new early return or the DEBUG branch, which is why codecov/patch fails.
4. Importance and alternatives
4.8 ms of a ~7 ms block at height 200k, still growing. There is no simpler fix; deleting the diagnostic entirely would also work but loses a useful log line.
5. Existing bot findings
- thepastaclaw "do not change versioned behavior in place": disagree on needing a v2 (nothing consensus-relevant changes); agree on making the diagnostic non-fatal.
- thepastaclaw "exercise the DEBUG path": agree; a test with a scoped DEBUG subscriber and one non-queued document covers both branches.
I will push both changes to this branch.
🤖 Posted autonomously by Claude on behalf of pasta.
The summary only runs when DEBUG logging is on, so a Drive error inside it would fail the block on a node with verbose logging and pass on one without. Log the error and carry on instead. Two tests with a scoped DEBUG subscriber cover the summary with and without history.
|
Pushed efd398d on top of the original commit:
Ready for human review. 🤖 Posted autonomously by Claude on behalf of pasta. |
The one with a document already covers the guarded path; the empty case adds nothing.
|
Two updates from review feedback:
🤖 Posted autonomously by Claude on behalf of pasta. |
PastaPastaPasta
added
the
ready for final review
PastaPastaPasta
changed the title
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Okay, let's get rid of that info.
3 participants
Issue being fixed or feature implemented
pool_withdrawals_into_transactions_queue_v1reads every withdrawal document the chain has ever produced, on nearly every block, and throws the result away.When nothing is queued — which is almost always — it calls
fetch_oldest_withdrawal_documents, which passeslimit: None. That deserializes every withdrawal document, groups them by status and sorts each group. The result feeds onetracing::debug!line and is otherwise discarded.Withdrawal documents are never removed, so the cost grows with chain history. Replaying mainnet with per-block phase timing (#4573), where "block time" below means the time drive-abci spends executing a block, proposal and finalize together, not the 2.5-minute interval between blocks at the tip:
Every other measured phase adds up to roughly 7,000 µs per block and stays flat with height; this call is the entire slope, and it was still climbing at 200k. Removing it makes a block at 200k about 1.7× faster to execute. A synced node does not notice, since 5 ms is nothing against a 150-second block interval; a node replaying history spends 41% of its time on it.
What was done?
Removed the historical withdrawal-document query, status counting, and diagnostic summary logs entirely. When no withdrawals are queued, the method returns immediately after its existing empty-queue message.
How Has This Been Tested?
cargo test --locked -p drive-abci --lib withdrawal— 176 passed.cargo clippy --locked -p drive-abci --lib --tests -- --no-deps -D warnings— passed.cargo fmt -p drive-abci -- --check— passed.The earlier guard-only implementation was also validated by replaying mainnet history from genesis to 424,981: every committed app hash matched across all 424,971 heights, and the final app hash matched an independent reference sync. That replay was not repeated for this deletion.
Breaking Changes
None. The removed query and counts only served the diagnostic summary.
Checklist:
For repository code-owners and collaborators only